Skip to content

coverage: Branch coverage support for let-else and if-let #124223

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
May 8, 2024
Merged

coverage: Branch coverage support for let-else and if-let #124223

merged 2 commits into from
May 8, 2024

Conversation

Zalathar
Copy link
Contributor

@Zalathar Zalathar commented Apr 21, 2024 •
edited
Loading

This PR adds branch coverage instrumentation for let-else and if-let, including let-chains.

This lifts two of the limitations listed at #124118.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) labels Apr 21, 2024
@Zalathar Zalathar mentioned this pull request Apr 21, 2024
Open
4 tasks
@Zalathar
Copy link
Contributor Author

Record if-let branches a bit later, to avoid unwrapping anything (diff).

@Zalathar Zalathar force-pushed the conditional-let branch 4 times, most recently from 9697dc5 to ae0c376 Compare April 29, 2024 03:23
@Zalathar
Copy link
Contributor Author

Originally I was holding this back until after #124154, both to make branch management easier, and to see whether further work on that PR would prompt changes to this one.

But while the future of #124154 remains a bit murky, I think it now makes sense to merge this PR as an independent improvement in its own right.

@Zalathar Zalathar marked this pull request as ready for review April 29, 2024 03:27
@rustbot
Copy link
Collaborator

rustbot commented Apr 29, 2024

Some changes occurred in match lowering

cc @Nadrieril

@Zalathar
Copy link
Contributor Author

r? compiler

compiler-errors
compiler-errors reviewed Apr 29, 2024
View reviewed changes
@Zalathar Zalathar force-pushed the conditional-let branch 2 times, most recently from 6e2e80a to 8db41bb Compare April 30, 2024 02:37
@Zalathar
Copy link
Contributor Author

After a rebase, I reintroduced an intermediate method instead of calling add_two_way_branch directly (diff).

This is indeed a nice improvement. In hindsight, a lot of my reasons for having done things the other way have now evaporated.

@bors
Copy link
Collaborator

bors commented Apr 30, 2024

☔ The latest upstream changes (presumably #124399) made this pull request unmergeable. Please resolve the merge conflicts.

@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented May 7, 2024

📌 Commit 7c87ad0 has been approved by compiler-errors

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 7, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 7, 2024
@matthiaskrgr
Rollup merge of rust-lang#124223 - Zalathar:conditional-let, r=compil…
7c5ac46 
…er-errors

coverage: Branch coverage support for let-else and if-let

This PR adds branch coverage instrumentation for let-else and if-let, including let-chains.

This lifts two of the limitations listed at rust-lang#124118.
@matthiaskrgr matthiaskrgr mentioned this pull request May 7, 2024
Closed
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 7, 2024
@matthiaskrgr
Rollup merge of rust-lang#124223 - Zalathar:conditional-let, r=compil…
6f38e9d 
…er-errors

coverage: Branch coverage support for let-else and if-let

This PR adds branch coverage instrumentation for let-else and if-let, including let-chains.

This lifts two of the limitations listed at rust-lang#124118.
@matthiaskrgr matthiaskrgr mentioned this pull request May 7, 2024
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request May 7, 2024
@bors
Auto merge of rust-lang#124856 - matthiaskrgr:rollup-xqxs21x, r=matth…
4883951 
…iaskrgr

Rollup of 3 pull requests

Successful merges:

 - rust-lang#124223 (coverage: Branch coverage support for let-else and if-let)
 - rust-lang#124571 (coverage: Clean up `mcdc_bitmap_bytes` and `conditions_num`)
 - rust-lang#124838 (next_power_of_two: add a doctest to show what happens on 0)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors
Copy link
Collaborator

bors commented May 7, 2024

⌛ Testing commit 7c87ad0 with merge 5486f0c...

@bors
Copy link
Collaborator

bors commented May 8, 2024

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 5486f0c to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 8, 2024
@bors bors merged commit 5486f0c into rust-lang:master May 8, 2024
@rustbot rustbot added this to the 1.80.0 milestone May 8, 2024
@bors bors mentioned this pull request May 8, 2024
Closed
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (5486f0c): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 2.5% [2.5%, 2.5%] 1
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 2.5% [2.5%, 2.5%] 1

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 1.1% [1.1%, 1.1%] 1
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -4.0% [-4.0%, -4.0%] 1
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) -1.4% [-4.0%, 1.1%] 2

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 2.4% [2.4%, 2.4%] 1
Regressions ❌
(secondary)		 4.1% [4.1%, 4.1%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 2.4% [2.4%, 2.4%] 1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 677.065s -> 676.569s (-0.07%)
Artifact size: 315.85 MiB -> 315.94 MiB (0.03%)

@Zalathar Zalathar deleted the conditional-let branch May 8, 2024 02:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@compiler-errors compiler-errors compiler-errors approved these changes

Assignees

@compiler-errors compiler-errors

Labels
A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.80.0
Development

Successfully merging this pull request may close these issues.
5 participants
@Zalathar @rustbot @bors @compiler-errors @rust-timer